Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Transport: Add HttpClientFactory support #1441

Merged
merged 11 commits into from
May 1, 2020
Merged

Transport: Add HttpClientFactory support #1441

merged 11 commits into from
May 1, 2020

Conversation

ealsur
Copy link
Member

@ealsur ealsur commented Apr 28, 2020 •
edited
Loading

This PR adds support for the user to provide a factory for HttpClient instances.

Scenarios

When building ASP.NET Core applications on NET Core 2.1/3, users can take advantage of IHttpClientFactory to reuse HttpClient instances across the application. This also allows HttpClient instances to be reused across CosmosClient instances.
Benefits of using IHttpClientFactory on ASP.NET Core applications: https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests#benefits-of-using-ihttpclientfactory

When building Blazor applications on WebAssembly, the applications use a preconfigured HttpClient instance that needs to be injected (reference https://docs.microsoft.com/en-us/aspnet/core/blazor/call-web-api?view=aspnetcore-3.1)

API

This PR adds HttpClientFactory to CosmosClientOptions:

public Func<HttpClient> HttpClientFactory { get; set; }

And WithHttpClientFactory to CosmosClientBuilder:

public CosmosClientBuilder WithHttpClientFactory(Func<HttpClient> httpClientFactory)

To provide a delegate that matches IHttpClientFactory.CreateClient.

The new API is not compatible with CosmosClientOptions.WebProxy and if the user tries to set both, there will be an ArgumentException.

Why not use IHttpClientFactory on the API?

To avoid a reference to Microsoft.Extensions.Http on the Cosmos DB SDK package unnecessarily.

Usage

In a Blazor WebAssembly application:

public static async Task Main(string[] args)
        {
            var builder = WebAssemblyHostBuilder.CreateDefault(args);
            builder.RootComponents.Add<App>("app");

            builder.Services.AddTransient(sp => new HttpClient { BaseAddress = new Uri(builder.HostEnvironment.BaseAddress) });

            builder.Services.AddHttpClient();

            builder.Services.AddSingleton<CosmosClient>(sp =>
            {
                var httpClientFactory = sp.GetService<IHttpClientFactory>();
                CosmosClientOptions cosmosClientOptions = new CosmosClientOptions();
                cosmosClientOptions.HttpClientFactory = httpClientFactory.CreateClient;
                cosmosClientOptions.ConnectionMode = ConnectionMode.Gateway;
                return new CosmosClient("<connection-string>", cosmosClientOptions);
            });

            await builder.Build().RunAsync();
        }

In an ASP.NET Core application:

public void ConfigureServices(IServiceCollection services)
{
    // other service configuration
    // adds in HttpClientFactory
    services.AddHttpClient();
    services.AddSingleton<CosmosClient>(sp =>
            {
                var httpClientFactory = sp.GetService<IHttpClientFactory>();
                CosmosClientOptions cosmosClientOptions = new CosmosClientOptions();
                cosmosClientOptions.HttpClientFactory = httpClientFactory.CreateClient;
                cosmosClientOptions.ConnectionMode = ConnectionMode.Gateway;
                return new CosmosClient("<connection-string>", cosmosClientOptions);
            });
}

Related to #1429. One more PR is required to fully support Blazor WebAssembly, but this is a good first step.

Tracking usage

The PR also adds a new Feature on the user agent to track and identify usage of the feature.

Type of change

  • New feature (non-breaking change which adds functionality)

@ealsur ealsur added the feature-request New feature or request label Apr 28, 2020
@ealsur ealsur self-assigned this Apr 28, 2020
@j82w j82w mentioned this pull request Apr 28, 2020
Closed
j82w
j82w previously approved these changes Apr 28, 2020
View reviewed changes
@ealsur ealsur mentioned this pull request Apr 28, 2020
Merged
1 task
@kirankumarkolli
Copy link
Member 

        httpClient.Timeout = (requestTimeout > this.requestTimeout) ? requestTimeout : this.requestTimeout;

Overriding these on CX given client seems like overstepping each other.
Any other possibilities?

Refers to: Microsoft.Azure.Cosmos/src/GatewayStoreModel.cs:111 in a4ae553. [](commit_id = a4ae553, deletion_comment = False)

kirankumarkolli
kirankumarkolli requested changes Apr 29, 2020
View reviewed changes
Copy link
Member

@kirankumarkolli kirankumarkolli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding CX given HttpClient seems like overstepping each other.
Any other possibilities?

@ealsur
Copy link
Member Author

ealsur commented Apr 29, 2020

@kirankumarkolli If the user is setting the RequestTimeout through ClientOptions and also providing an HttpClient from the factory, I don't see the issue of configuring the provided instance.

We have 2 choices, either assume the user would configure the timeout, or not.

If we target the most common scenarios for users to use this API, which are described in the Description, then the user would not be configuring the timeout there, but they could be setting the timeout on the ClientOptions and I would expect the Options to be honored.

If the user is indeed configuring a custom timeout on the HttpClientFactory, and they expect that to be honored, we just tell them that they can configure that through the ClientOptions, as they can do right now.

@ealsur ealsur mentioned this pull request Apr 29, 2020
Open
@ealsur
Copy link
Member Author

ealsur commented Apr 29, 2020

@kirankumarkolli I added a new Feature flag on the UserAgent, so we can identify when a user is using their own HttpClientFactory

@ealsur ealsur merged commit 5024c15 into master May 1, 2020
@ealsur ealsur deleted the users/ealsur/httpfactory branch May 1, 2020 15:24
@FVilevski FVilevski mentioned this pull request May 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirankumarkolli kirankumarkolli kirankumarkolli approved these changes

@j82w j82w j82w left review comments

@bchong95 bchong95 Awaiting requested review from bchong95

@FabianMeiswinkel FabianMeiswinkel Awaiting requested review from FabianMeiswinkel FabianMeiswinkel is a code owner

@khdang khdang Awaiting requested review from khdang khdang is a code owner

@kirillg kirillg Awaiting requested review from kirillg kirillg is a code owner

@sboshra sboshra Awaiting requested review from sboshra sboshra is a code owner

Assignees

@ealsur ealsur

Labels
feature-request New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ealsur @kirankumarkolli @j82w